I am not sure we want this behavior. I think the logic should be:

• If InstanceUID is specified in the config then use it. This is needed for the Supervisor use case.
• If InstanceUID is NOT specified in the config then generate a UUIDv7 and use it. We should not use the value of ServiceInstanceIDKey attribute as the value of OpAMP instance UID. ServiceInstanceIDKey is not guaranteed to be globally unique. If we want, we can additionally persist the value of generated UUIDv7, so that we reuse the value on restarts. That can be a new feature we can consider for opampextention.

Hm, that would be a much more impactful change for people using the extension without the supervisor, but I can see value in not getting server implementations used to both values matching.

I don't think the first part would be necessary since (if I'm not mistaken) the supervisor always sets both instance_uid and service.instance.id in the Collector resource attributes, so I guess it would just amount to removing this else branch entirely.

it would just amount to removing this else branch entirely.

yes, that's what I was suggesting.

@evan-bradley @andykellr what do you think?

Why is this removed? Looks weird. I think we should assert on some expected service instance ID here, no?

Hm, you're right. I removed it because the test was failing and I thought it was testing the extension (in which case we expect the instance ID to be random), but since this seems to be for the supervisor I would expect it to match the instance UID...

The usual error seems to be "could not get bootstrap info from the Collector: collector's OpAMP client never connected to the Supervisor", which I can't easily relate to any of my changes. I've been trying to debug these tests for a while but I'm honestly somewhat at a loss, mostly because all the useful logic happens in a different process, so the debugger never gets attached. Do you have advice on where the discrepancy could come from?

I tried some stuff locally and I hit a similar problem to you, @jade-guiton-dd. I could run only this specific test with go test -tags=e2e -run 'TestSupervisorAgentDescriptionConfigApplies' -count=1 -v . (in the cmd/opampsupervisor folder) though.

It seems like require.Subset ends up comparing values with reflect.DeepEqual (see https://github.com/stretchr/testify/blob/v1.11.1/assert/assertions.go#L71) and I think the protobuf types might somehow be interfering with the comparison now, but I don't understand why.

I did some local changes to convert []*protobufs.Keyvalue to a map[string]string and I got a proper test failure due to mismatch in expected values:

diff --git a/cmd/opampsupervisor/e2e_test.go b/cmd/opampsupervisor/e2e_test.go
index 5efe2bc722c..8a0ce229c19 100644
--- a/cmd/opampsupervisor/e2e_test.go
+++ b/cmd/opampsupervisor/e2e_test.go
@@ -1237,6 +1237,7 @@ func TestSupervisorAgentDescriptionConfigApplies(t *testing.T) {
 	expectedDescription := &protobufs.AgentDescription{
 		IdentifyingAttributes: []*protobufs.KeyValue{
 			stringKeyValue("client.id", "my-client-id"),
+			stringKeyValue("service.instance.id", uuid.UUID(ad.InstanceUid).String()),
 			stringKeyValue("service.name", command),
 			stringKeyValue("service.version", version),
 		},
@@ -1248,8 +1249,14 @@ func TestSupervisorAgentDescriptionConfigApplies(t *testing.T) {
 		},
 	}
 
-	require.Subset(t, ad.AgentDescription.IdentifyingAttributes, expectedDescription.IdentifyingAttributes)
-	require.Subset(t, ad.AgentDescription.NonIdentifyingAttributes, expectedDescription.NonIdentifyingAttributes)
+	actualIdentifyingAttributes := keyValuesToStringMap(ad.AgentDescription.IdentifyingAttributes)
+	require.Subset(t, actualIdentifyingAttributes, keyValuesToStringMap(expectedDescription.IdentifyingAttributes))
+	require.Contains(t, actualIdentifyingAttributes, "service.instance.id")
+	_, err = uuid.Parse(actualIdentifyingAttributes["service.instance.id"])
+	require.NoError(t, err)
+
+	actualNonIdentifyingAttributes := keyValuesToStringMap(ad.AgentDescription.NonIdentifyingAttributes)
+	require.Subset(t, actualNonIdentifyingAttributes, keyValuesToStringMap(expectedDescription.NonIdentifyingAttributes))
 
 	time.Sleep(250 * time.Millisecond)
 }
@@ -1265,6 +1272,14 @@ func stringKeyValue(key, val string) *protobufs.KeyValue {
 	}
 }
 
+func keyValuesToStringMap(kvs []*protobufs.KeyValue) map[string]string {
+	out := make(map[string]string, len(kvs))
+	for _, kv := range kvs {
+		out[kv.Key] = kv.Value.GetStringValue()
+	}
+	return out
+}
+
 // Creates a Collector config that reads and writes logs to files and provides
 // file descriptors for I/O operations to those files. The files are placed
 // in a unique temp directory that is cleaned up after the test's completion.

When I run the test above with the assertion that this PR removed on the service.instance.id plus this patch applied I get this:

(...)
    e2e_test.go:1253:
                Error Trace:    /Users/douglas/code/opentelemetry-collector-contrib/cmd/opampsupervisor/e2e_test.go:1253
                Error:          map[string]string{"client.id":"my-client-id", "service.instance.id":"6b1f4a09-081d-4d28-ae2c-85c59ab349fe", "service.name":"otelcontribcol", "service.version":"0.146.0-dev"} does not contain map[string]string{"client.id":"my-client-id", "service.instance.id":"019cd884-2d8a-72b3-b7ab-00a3a0eb0623", "service.name":"otelcontribcol", "service.version":"0.146.0-dev"}
                Test:           TestSupervisorAgentDescriptionConfigApplies
(...)

Right, so that confirms that the supervisor is not properly setting service.instance.id at the Collector level.

I originally assumed it did because of this template, but after looking deeper into it, I'm not actually sure how ResourceAttributes is filled. It definitely contains the explicitly-specified values from agent.description.identifying_attributes, but I'm not sure if service.instance.id is ever added in there.

So I guess there are two options:

• either keep the existing behavior in the Supervisor where service.instance.id is always set to the OpAMP instance UID, which would require modifying the code to make sure the resource attribute is always overwritten
• apply the same "radical" change Tigran proposed in the Supervisor as well, and completely decorrelate them (which amounts to removing a test case in the way I did above)

Since for some reason the test is still failing for me locally, I tried pushing a new version with a modified composeExtraTelemetryConfig that adds the test line back in and always sets service.instance.id to the instance UID (unless explicitly set in s.agentDescription)

Okay, I figured it out.

• The reason why the tests were failing locally for me with that different error is that the bootstrap Collector tries to start its Prometheus endpoint, but I had a leftover Collector running on my system, causing a port conflict and preventing the bootstrap Collector from starting.
• The reason why my latest change apparently failed to set service.instance.id is because composeExtraTelemetryConfig is not called when composing the configuration for the bootstrap Collector. So I instead decided to set the attribute directly inside templates/opampextension.yaml. This seems to fix the test, which is also simplified a bit from the patch you suggested above. I am assuming this will compose gracefully with the "extra telemetry" configuration.

Awesome. I'm happy that this is fixed and that you could use a simpler patch 😃

I will do a full review pass in a bit. Great work!

@douglascamata did you find the time to review this?

I’ll find some time to check it out later today or tomorrow. 👍

I am confused by why we have 2 fields now agentInstanceID and instanceID. What's the difference other than the field type? I expect the agent to have one instance id. Can these fields have 2 different id values? When does that happen?

From what I understood through the changelog entry, agentInstanceID represents the Collector's instance ID and instanceID represents the OpAMP extension's instance_uid. Both will match unless instance_uid is explicitly configured in the opampextension. Please correct me if I'm wrong, @jade-guiton-dd.

It sounds a bit confusing, yes. Maybe we could have a comment explaining why these two fields with very similar names exist?

The two concepts have always had confusingly similar names to be honest, but I can rename the fields and add comments if that makes things clearer.

Note that the change Tigran suggested earlier makes it so that they won't usually match. They will only match if the user (or the supervisor) manually sets both (with the instance_uid config and service::telemetry::resource) to the same specific value.

I ended up renaming agentType, agentVersion, agentInstanceID, and instanceID to serviceName, serviceVersion, serviceInstanceID, and instanceUID respectively, tell me if that helps clear things up.

OK, renaming definitely helps understand what's going on.

I think this is the desirable behavior now.

Can you please also describe in a bit more details in the PR description the impact of the change. The description you have in changelog subtext is great, I think it can be used in the PR description too.